-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove ReportsProcessed DO #433
Conversation
a9aafd8
to
1066290
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see that we can basically rip this out. However I think I see at least one regression: The Leader no longer checks for replays in time to reject the reports during an aggregation job.
Also, and this is somewhat separate so feel free to punt: The ReportsProcessed
DO instances are self-cleaning in the sense that, on creation, an alarm is set that deletes the instance some time later. Now would be a good time to implement this for AggregateStore
as well, since we mainly just need to copy-paste the code from the reports_processed
module to aggregate_store
.
30f35ab
to
f6a83f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! One more question about the code itself, other comments are just related to documentation.
f6a83f3
to
9571ad1
Compare
9571ad1
to
15f5930
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall, but I think this needs a few minor changes. I've also bumped some comments that we missed in the previous round.
Also, I think we should delete this outdated comment we noticed on Tuesday (this caused some confusion about if/when the Leader needs to check for replays): https://github.com/cloudflare/daphne/blob/main/daphne_worker/src/roles/leader.rs#L105-L109
d4d378c
to
8231f9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦆 great work
8231f9e
to
ec84c0b
Compare
No description provided.